feat: explicit proxy configuration for the CLI - #698

TLS verification is unconditionally disabled for all API traffic. insecureSkipVerify is hardcoded true at this call site, so the CLI never verifies the Octopus server certificate and --ignore-ssl-errors is effectively always on (the PR description acknowledges this in open question 6). Now that the insecure flag is an explicit parameter, this is the natural moment to plumb the real setting through (default false) or at minimum open the follow-up issue before merging — a MITM on any network path to the server can silently capture the API key sent on every request.

Actioned in 884abe2. Confirmed the finding first: on this branch NewClientFactoryFromConfig passed a literal true, so every *http.Transport the factory built carried InsecureSkipVerify: true and no command verified the server certificate.

Before: verification off for all API traffic, unconditionally, with no way to turn it on. After: verification on, and turning it off is an explicit opt-in via one of

• OCTOPUS_IGNORE_SSL_ERRORS=true
• octopus config set IgnoreSslErrors true
• octopus login --ignore-ssl-errors (one login only)

Plumbing, rather than just deleting the true: new IgnoreSslErrors config key defaulted to false in setDefaults, bound to OCTOPUS_IGNORE_SSL_ERRORS in bindEnvironment, read at the call site through apiclient.IgnoreSslErrorsFromConfig(). config set rejects a non-boolean value for it, because viper reads yes back as false and a user who thinks they turned verification off deserves an error rather than a setting that silently does nothing. The key is also listed by config list -f json and offered by the config get/config set pickers.

Tests: TestNewClientFactoryFromConfig_TlsVerification (table: default, explicit false, true, "true") asserts on the transport the factory actually hands out; TestSetup_DefaultsToVerifyingTheServerCertificate and TestSetup_BindsTheIgnoreSslErrorsEnvironmentVariable cover the key itself.

Verified end to end with a binary built from 884abe2 against https://self-signed.badssl.com:

default:                          tls: failed to verify certificate: x509: certificate signed by unknown authority
OCTOPUS_IGNORE_SSL_ERRORS=true:   invalid character '<' looking for beginning of value   (past TLS, failing on the HTML body)

This is a breaking change and it is deliberate — the whole point is that the old behaviour was silently insecure. Anyone on a self-signed or internal-CA certificate who is not currently opted out will start getting x509: certificate signed by unknown authority after upgrading. The README section I added says exactly that, recommends fixing the trust store first, and flags the opt-out as trust-the-network-path only. Worth calling out in the release notes; your call whether that is enough or whether this needs to land on its own release boundary.

While extending this switch: config list -f json currently hard-errors for anyone who has run octopus login, because both loginWithApiKey and loginWithOpenIdConnect always write AccessToken to the config file (even as ""), and accesstoken has no case here (nor a ConfigData field) so it falls into default: return fmt.Errorf(...). Verified empirically on this branch: a config file containing accesstoken makes listRun return the key 'accesstoken' is not a supported config option and print nothing. ShowOctopus hits the same default. Pre-existing, but this PR touches the switch and adds a masked accesstoken entry via configFile.Set(constants.ConfigAccessToken, "***") above, so it is worth fixing here or in a fast follow.

Actioned in 82f320d, extended in 884abe2.

The finding holds as written. On origin/main the ConfigData struct has no AccessToken and no ShowOctopus field and the switch has neither case, so both keys fall into default: — accesstoken is written by every octopus login (as "" for the OIDC/api-key path that does not use it), which is what made it hit everyone who had logged in.

82f320d adds the accesstoken and showoctopus cases and fields. 884abe2 adds ignoresslerrors alongside them, since that key is new in this PR and would otherwise have reintroduced exactly the same failure the moment someone set it.

Verified with a binary built from 884abe2 against a config file containing accesstoken, apikey, ignoresslerrors, proxyurl and url:

{
  "accesstoken": "***",
  "apikey": "***",
  "editor": "",
  "host": "https://octopus.example.com",
  "ignoresslerrors": "true",
  "noprompt": "",
  "outputformat": "",
  "proxyurl": "http://octo:[email protected]:3128",
  "showoctopus": "",
  "space": ""
}

exit 0, where the same file on main returns the key 'accesstoken' is not a supported config option.

Residual, unchanged: the default: return fmt.Errorf(...) is still there, so the next config key anyone adds without touching this switch breaks -f json the same way. Enumerating ConfigData from the constants, or skipping unknown keys instead of erroring, would make that structural — out of scope here, and it needs a call on whether an unknown key in the file should be an error at all.

Behavior narrowing vs the old code: the removed code applied InsecureSkipVerify to any non-nil client, including one with a nil Transport (if httpClient.Transport == nil { httpClient.Transport = &http.Transport{} }). The new code only handles *apiclient.SpinnerRoundTripper; for a non-nil client with a nil or any other transport, --ignore-ssl-errors is now silently dropped (the new test even enshrines this). Unreachable via NewClientFactoryFromConfig/the stub today, but any factory implementation that returns a plain &http.Client{} gets a flag that does nothing, with no warning. Consider handling httpClient.Transport == nil explicitly (build the proxy-aware transport, as the nil-client branch does) so the previously-supported case keeps working.

Actioned in 4492488.

The narrowing was real: the removed code did if httpClient.Transport == nil { httpClient.Transport = &http.Transport{} } before setting TLSClientConfig, and the first version of ConfigureHttpClient only matched *apiclient.SpinnerRoundTripper, so a non-nil client with a nil transport silently lost --ignore-ssl-errors (and got http.DefaultTransport, which knows nothing about the CLI's proxy settings either — so it lost the proxy too, which is arguably the worse half).

ConfigureHttpClient now has an explicit httpClient.Transport == nil branch that builds the proxy-aware transport, the same way the nil-client branch does, and assigns it into the caller's client. The test that enshrined the old behaviour is replaced by TestConfigureHttpClient/"gives a client with no transport a proxy aware one", which asserts both halves: InsecureSkipVerify is set, and transport.Proxy resolves to the configured proxy.

Transport ordering after the change: nil client → build; nil transport → build and assign; *SpinnerRoundTripper → rebuild Next; anything else → left alone (covered by TestConfigureHttpClient/"leaves a transport it does not own alone", which is the mock case).

octopus config get ProxyUrl (now offered in this interactive picker) prints the value raw via configFile.GetString(key) — including an embedded user:password. That contradicts the redaction added to config list and the PR's stated invariant that the password is never echoed. It matches the existing (also unredacted) config get ApiKey precedent, but since this PR adds the redaction machinery, consider running the value through apiclient.RedactProxyUrl in getRun when the key is ProxyUrl.

Actioned in 0ac4906. getRun now runs the value through apiclient.RedactProxyUrl when the key is ProxyUrl (matched with strings.EqualFold, since the key arrives in whatever case the user typed).

Verified with a binary built from 884abe2, config file holding "proxyurl": "http://octo:[email protected]:3128":

$ octopus config get ProxyUrl --no-prompt
http://octo:[email protected]:3128

Deliberately not extended to ApiKey/AccessToken: config get ApiKey printing the key raw is existing behaviour that something may well be scripted against, and changing it is a separate decision from "this PR should not add a new way to leak a secret".

Silent failure mode: if OCTOPUS_PROXY_PASSWORD is set but OCTOPUS_PROXY_USERNAME is empty (unset, or typo'd var name), the credentials are dropped with no diagnostic — the request goes to the proxy unauthenticated and the user gets a bare 407 with no hint that the CLI ignored their password. Consider warning (or erroring) when Password != "" && Username == "" in ProxySettingsFromConfig/ProxyFunc.

Actioned in a5ee140, as an error rather than a warning.

Placed in the returned ProxyFunc closure rather than in ProxySettingsFromConfig, so it only fires when it actually matters — a proxy is in play for this request and the proxy url carries no credentials of its own:

if s.Password != "" && s.Username == "" && proxyUrl.User == nil {
    return nil, fmt.Errorf("%s is set but %s is empty, so the proxy credentials cannot be used", ...)
}

A stray OCTOPUS_PROXY_PASSWORD left in someone's shell profile therefore does not break a direct connection or a NO_PROXY-matched host, and it does not break the case where the password came from the url itself.

Error over warning because there is no good reading of it: the user asked for authenticated proxying, and the alternative is a 407 whose cause is invisible. It names both variables so a typo'd OCTOPUS_PROXY_USERNAME is self-diagnosing.

Tests: TestProxySettings_ProxyFuncRejectsAPasswordWithNoUsername and TestProxySettings_ProxyFuncIgnoresAPasswordWithNoProxy. Verified end to end with a binary built from 884abe2:

$ OCTOPUS_PROXY=http://proxy.invalid:3128 OCTOPUS_PROXY_PASSWORD=s3cret octopus space list --no-prompt
cannot get endpoint https://octopus.example.com/api/ from server. failure from http client
Get "https://octopus.example.com/api/": OCTOPUS_PROXY_PASSWORD is set but OCTOPUS_PROXY_USERNAME is empty, so the proxy credentials cannot be used

so it does survive the SDK's error wrapping and reaches the terminal.

Two smaller notes on this branch: (1) as long as client_factory.go:123 hardcodes insecureSkipVerify=true, spinnerRoundTripper.Next is already an insecure proxy-aware transport, so this rebuild is a functional no-op that only resets the connection pool — it becomes meaningful once the factory plumbs the real setting (fine to keep for that reason, but worth a comment saying so). (2) It mutates the factory's shared client in place, so after login --ignore-ssl-errors every subsequent request in the process skips TLS verification, not just the login probe — intended for a one-shot CLI, but a trap for any longer-lived embedding.

Both notes were correct. (1) is now obsolete and (2) is documented rather than changed.

(1) — resolved by 884abe2. The premise was the hardcoded insecureSkipVerify=true in NewClientFactoryFromConfig, which made this rebuild a pool reset and nothing more. That hardcode is gone: the factory reads the real IgnoreSslErrors setting, so with the setting off and --ignore-ssl-errors passed, spinnerRoundTripper.Next genuinely goes from a verifying transport to a non-verifying one. The comment you asked for was added (aaaf8a6) and has since been removed as stale, since it now describes the opposite of what happens.

Also in 884abe2: ConfigureHttpClient ORs the IgnoreSslErrors config key into its parameter, so the standing opt-out applies to login too. Without that, login would have been the one command that still verified when the user had opted out globally — a confusing failure, because login is exactly the command you run first. Covered by TestConfigureHttpClient/"honours the IgnoreSslErrors config key without the flag" and .../"verifies the server certificate by default".

(2) — left as is, with the comment. The in-place mutation is deliberate and the code now says so:

// Note that this mutates the factory's shared client rather than cloning it, so
// --ignore-ssl-errors outlives the login probe and applies to every later request
// in the process: fine for a one-shot CLI, a trap for any longer-lived embedding.

Cloning would be the defensive fix, but it is not free: testLogin builds a ClientFactory from this same client and the spinner round-tripper is shared state by design, so a clone changes what the spinner sees and what TestConfigureHttpClient can assert about identity. It also buys nothing today — main() exits after one command. My read is that this stays as it is until something actually embeds the CLI, at which point the whole factory-returns-a-shared-client design needs the look, not just this line. Happy to be wrong if you know of an embedding: is there one?

parseProxyUrl re-implements the private parseProxy in golang.org/x/net/http/httpproxy (same err != nil || Scheme == "" || Host == "" + "http://"+addr fallback), but the parsed result is then discarded and the raw string is handed to httpproxy to parse again with its own copy of the rules. They match today; if either side changes (e.g. httpproxy tightens scheme handling), validation here and actual resolution there can drift apart — a url this function accepts could be silently ignored by httpproxy, which is exactly the failure the comment above says this validation exists to prevent. Consider assigning the parsed (normalized) url into config.HTTPProxy/HTTPSProxy via .String() so one parse is authoritative.

Actioned in 1b92f89, taking the suggested shape: the parsed url is now what httpproxy gets.

parsed, err := parseProxyUrl(s.Url)
if err != nil {
    return nil, err
}
config.HTTPProxy = parsed.String()
config.HTTPSProxy = parsed.String()

So the double parse is still there, but it is no longer two independent decisions about the same string: httpproxy parses a url that has already been normalized (scheme filled in), and the drift you describe — we accept host:port, httpproxy rejects it and silently connects direct — can't happen, because httpproxy never sees the un-schemed form.

On the dependency: golang.org/x/net was already in go.mod as an indirect dependency at v0.57.0, so using httpproxy costs nothing but promoting it to direct, which is the one-line go.mod change in this PR. No new module.

Residual I did not fix: parseProxy is still private, so the fallback rule (err != nil || Scheme == "" || Host == "" → prepend http://) is still duplicated as a rule, just no longer applied twice to the same input. Removing the duplication entirely would mean either vendoring the function or dropping httpproxy and reimplementing NO_PROXY matching, which is the part of that package genuinely worth having.